Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Automated inline review completed.

Review effort: medium (246 changed lines across 13 files).
Submitted 3 inline comments.

Reviewed commit 9b6cf96.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Automated inline review completed.

Review effort: high (397 changed lines across 15 files).
Submitted 7 inline comments.

Reviewed commit e718e38.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

This update addresses all six issues raised in the previous review round:

1. IVF reorder threshold — Threshold is now applied after exact-distance reorder, with FilterDatasetByThreshold capping at topk_. The candidate pool is retained at full size when threshold is present.
2. Non-finite threshold validation — ValidateSearchThreshold rejects NaN/Inf in SearchWithRequest paths, and index_impl.h validates inside SAFE_CALL for consistent error handling.
3. Allocator preservation — FilterDatasetByThreshold accepts an Allocator*, allocates through it, and sets Owner(true, allocator). HNSW and SINDI pass their caller allocator; a regression test verifies balanced deallocation.
4. route_buckets_only threshold — IVF now explicitly rejects threshold with disable_bucket_scan via CHECK_ARGUMENT.
5. IVF reasoning report — AttachReasoningReport is called after FilterDatasetByThreshold in the reorder path.
6. Pyramid/SIMQ threshold — Both now parse and apply threshold via FilterDatasetByThreshold with their allocators and k-capping.

Additional observations:

• The #include <unordered_set> addition in test_brute_force.cpp fixes a missing-include bug in the existing code (it was used but not included).
• ParseSearchThreshold now validates JSON type via IsNumber() before calling GetFloat(), preventing silent coercion of string values.
• The SetOFFFirstUsed() call in HGraph when threshold filtering empties the result is a correct fix to maintain iterator filter state consistency.
• FilterDatasetByThreshold correctly preserves statistics and reasoning from the input dataset.

One minor suggestion was posted inline about the two-pass iteration in FilterDatasetByThreshold.

Automated inline review completed.

Review effort: high (1177 changed lines across 27 files).
Submitted 4 inline comments.

Reviewed commit b35bbc3.

🟡 Not ready to approve

The current top-level threshold validation in IndexImpl appears to change legacy (unsupported) index behavior and the new threshold utility contains dead/unreachable code that should be corrected.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

    if (result_count == 0) {
        result->ExtraInfos(nullptr)->ExtraInfoSize(0);
    } else if (extra_infos == nullptr and extra_size > 0 and input_extra_infos != nullptr) {
        result->ExtraInfos(nullptr)->ExtraInfoSize(extra_size);
    }

    ValidateThresholdParameters(const std::string& parameters) const {
        try {
            ParseSearchThreshold(parameters);
            return {};
        } catch (const VsagException& e) {

Threshold filtering is supported by the maintained indexes: BruteForce, HGraph, IVF, Pyramid,
SINDI, and SIMQ. HNSW and DiskANN are deprecated and are explicit unsupported
non-goals; this option does not add or change behavior for either legacy index.

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Automated inline review completed.

Review effort: high (1276 changed lines across 27 files).
Submitted 6 inline comments.

Reviewed commit eb2d560.

Thanks for this comprehensive PR. The threshold filtering feature is well-implemented across all six maintained indexes with thorough test coverage (109 assertions in 9 threshold-specific cases, plus full unit suite passing).

I reviewed the full diff and found only minor suggestions beyond what has already been addressed in the 30 existing review threads:

1. [suggestion] The traversal_priority and is_result_distance_eligible helpers are duplicated in basic_searcher.cpp and parallel_searcher.cpp. Consider extracting to a shared header.

2. [note] An unreachable defensive branch in FilterDatasetByThreshold (the else if for extra_infos == nullptr).

The core logic — non-finite distance filtering, threshold validation at the IndexImpl layer, IVF reorder/non-reorder paths, HGraph iterator page consumption, and the graph searcher traversal changes — all look correct. The GetNumElements() → GetDim() fix in AttachReasoningReport is a good catch.

No blocking issues found.

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.

Threshold filtering is supported by the maintained indexes: BruteForce, HGraph, IVF, Pyramid,
SINDI, and SIMQ. HNSW and DiskANN are deprecated and are explicit unsupported
non-goals; this option does not add or change behavior for either legacy index.

              BitsetPtr invalid = nullptr) const override {
        auto threshold_validation = ValidateThresholdParameters(parameters);
        if (not threshold_validation.has_value()) {
            return tl::unexpected(threshold_validation.error());
        }

    int64_t result_count = 0;
    for (const auto& [distance, _] : reranked) {
        if (result_count >= k) {
            break;
        }
        if (not threshold.has_value() or
            (std::isfinite(distance) and distance <= threshold.value())) {
            ++result_count;
        }
    }

        if (ptr != nullptr) {
            allocations_[ptr] = size;
            allocated_bytes_ += size;
            allocation_count_ += 1;
        }

    uint64_t
    AllocationCount() const {
        std::scoped_lock lock(mutex_);
        return allocation_count_;
    }

    uint64_t allocation_count_{0};

Code Review Summary

All 14 previous review comments have been addressed and resolved in commit 04228c00. The current code state looks correct across all reviewed dimensions:

Correctness

• Threshold filtering is correctly applied after reorder in IVF (exact distances), before reorder in the bucket search path (approximate distances with enable_reorder=false), and via FilterDatasetByThreshold in SINDI/Pyramid
• Non-finite distance handling in basic_searcher and parallel_searcher correctly uses traversal_priority() for heap ordering and is_result_distance_eligible() for result inclusion
• FilterDatasetByThreshold properly handles the result_count == 0 case (null ExtraInfos, zero ExtraInfoSize)
• AllocateThresholdArray correctly returns nullptr for zero-count and throws std::bad_alloc on allocator failure
• ValidateThresholdParameters is called before empty-index checks in all 5 KnnSearch overloads and SearchWithRequest
• IVF route_buckets_only path explicitly rejects threshold with CHECK_ARGUMENT
• HGraph iterator path properly consumes threshold-rejected candidates via SetPoint and retries internally on empty pages
• hgraph_build.cpp defensively checks Empty() before Top() on route graph search results

Test Coverage

• BruteForce: threshold filtering, non-finite distances, NaN base vectors, IP metric, allocator ownership, empty index validation, malformed JSON rejection
• HGraph: iterator threshold consumption, non-finite entry distances, non-finite candidate filtering, infinity preservation without threshold, build tolerance for non-finite route probes
• Searcher unit tests: non-finite distance bridge traversal for both basic and parallel searchers
• FilterDatasetByThreshold unit tests: allocator ownership, extra-info preservation, non-finite input, allocation failure

No New Issues Found

The implementation is consistent across all 6 maintained indexes. No blocking issues, no suggestions worth flagging.

Code Review Summary (commit 04228c0)

All 14 previous review comments have been addressed and resolved. After thorough review of all 30 changed files, no new issues were found. The implementation is consistent across all 6 maintained indexes (BruteForce, HGraph, IVF, Pyramid, SINDI, SIMQ).

Key areas verified:

• Threshold filtering is correctly applied after reorder in IVF, and before reorder in bucket search path
• Non-finite distance handling in basic_searcher/parallel_searcher correctly uses traversal_priority() and is_result_distance_eligible()
• FilterDatasetByThreshold properly handles result_count==0, allocator ownership, and extra-info preservation
• ValidateThresholdParameters is called before empty-index checks in all KnnSearch overloads
• IVF route_buckets_only explicitly rejects threshold
• HGraph iterator path consumes threshold-rejected candidates via SetPoint
• hgraph_build.cpp defensively checks Empty() before Top()

Test coverage includes BruteForce, HGraph, searcher unit tests, and FilterDatasetByThreshold unit tests covering threshold filtering, non-finite distances, NaN, IP metric, allocator ownership, and allocation failure.

Code Review Summary (commit 04228c0)

All 14 previous review comments have been addressed and resolved. After thorough review of all 30 changed files, no new issues were found.

Key areas verified:

• Threshold filtering correctly applied after reorder in IVF, before reorder in bucket search path
• Non-finite distance handling in basic_searcher/parallel_searcher uses traversal_priority() and is_result_distance_eligible()
• FilterDatasetByThreshold handles result_count==0, allocator ownership, extra-info preservation
• ValidateThresholdParameters called before empty-index checks in all KnnSearch overloads
• IVF route_buckets_only explicitly rejects threshold
• HGraph iterator path consumes threshold-rejected candidates via SetPoint
• hgraph_build.cpp defensively checks Empty() before Top()

Test coverage: BruteForce, HGraph, searcher unit tests, FilterDatasetByThreshold unit tests.

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.

SINDI, and SIMQ. HNSW and DiskANN are deprecated and are explicit unsupported
non-goals; this option does not add or change behavior for either legacy index.

        auto threshold_validation = ValidateThresholdParameters(parameters);
        if (not threshold_validation.has_value()) {
            return tl::unexpected(threshold_validation.error());
        }
        CHECK_QUERY_RETURN_EMPTY_DATASET(query);

        SAFE_CALL(ValidateSearchThreshold(request.threshold_);
                  if (GetNumElements() == 0 && !this->ShouldSkipEmptyCheck(request.params_str_)) {
                      return DatasetImpl::MakeEmptyDataset();
                  } return this->inner_index_->SearchWithRequest(request));

    int64_t result_count = 0;
    for (const auto& [distance, _] : reranked) {
        if (result_count >= k) {
            break;
        }
        if (not threshold.has_value() or
            (std::isfinite(distance) and distance <= threshold.value())) {
            ++result_count;
        }
    }

Automated inline review completed.

Review effort: high (1623 changed lines across 34 files).
Submitted 2 inline comments.

Reviewed commit c9bceda.

[note] SINDI threshold comparison uses 1.0F + dist which accounts for the internal dist = -ip representation. This is correct for IP metric but would be incorrect for L2. If SINDI only supports IP, consider documenting this assumption or adding a metric guard to prevent future misuse when L2 support is added. (src/algorithm/sindi/sindi.cpp:736, src/datacell/sparse_term_datacell.cpp:75,124)

[note] The is_result_distance_eligible<KNN_SEARCH> function in searcher_utils.h rejects non-finite distances when a threshold is set, but only checks std::isfinite(distance). This means -inf distances (which are "better" than any finite distance) are also rejected. While -inf is unusual in practice, if it were to occur (e.g., from a degenerate vector), it would be incorrectly excluded from results. Consider using std::isnan instead of not std::isfinite for the threshold-aware check, or document that -inf is intentionally excluded. (src/impl/searcher/searcher_utils.h:30-31)

[note] In flat_bucket_searcher.cpp, the KNN path checks param.distance_threshold but the RANGE_SEARCH path does not. This is intentional since threshold is a KNN-only feature, but the asymmetry could be confusing to future readers. Consider adding a brief comment in the RANGE_SEARCH path noting that threshold filtering is intentionally not applied here. (src/algorithm/ivf/flat_bucket_searcher.cpp:71-72,99+)

Overall Review

This PR adds optional inclusive distance threshold filtering to KNN search across all 6 maintained indexes (BruteForce, HGraph, IVF, Pyramid, SINDI, SIMQ). The implementation is thorough and well-tested after multiple rounds of review.

What works well

1. Consistent API: std::optional<float> threshold flows through SearchRequest → InnerSearchParam → individual index implementations. All indexes use the same ParseSearchThreshold/ValidateSearchThreshold utilities.

2. NaN/inf safety: All threshold comparison paths use std::isfinite checks before comparing distances. The filter_search_result_by_threshold helper and is_result_distance_eligible template both guard against unordered float comparisons.

3. Exception safety: FilterDatasetByThreshold attaches each allocation to the owning Dataset immediately, preventing leaks on allocation failure. The AllocateThresholdArray helper provides a unified allocation path.

4. Test coverage: Every maintained index has threshold-specific tests covering: basic filtering, NaN handling, non-finite rejection, empty results, IP metric, iterator paths (HGraph), reorder paths (IVF), and allocator ownership.

5. Review iteration quality: All issues from previous review rounds (iterator consumption, IVF reasoning report ordering, SIMQ statistics accuracy, empty-index JSON validation, NaN-safe heap filtering, allocator ownership) have been properly addressed.

Minor observations (non-blocking)

1. SINDI 1.0F + dist pattern (sindi.cpp:736, sparse_term_datacell.cpp:75,124): Correctly accounts for the internal dist = -ip representation, but applies unconditionally regardless of metric type. If SINDI ever supports L2, this would silently produce wrong results.

2. -inf rejection (searcher_utils.h:30-31): is_result_distance_eligible<KNN_SEARCH> rejects all non-finite distances when threshold is set, including -inf which is technically "better" than any finite distance. This is unlikely to occur in practice but worth noting.

3. flat_bucket_searccher.cpp asymmetry: The KNN path has threshold filtering but the RANGE_SEARCH path does not — intentional but could benefit from a brief comment.

Verdict

The implementation is correct, well-tested, and all previously identified issues have been resolved. Approved.